Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

477 add progress hook to optimize #592

Merged
merged 15 commits into from
Aug 27, 2024
Merged

477 add progress hook to optimize #592

merged 15 commits into from
Aug 27, 2024

Conversation

sabinala
Copy link
Contributor

Adding progress hook to optimize. Should output the number of function evaluations so that a progress bar can be made in Terarium from this number divided by: maxfeval * (maxiter + 1).

@sabinala sabinala added the WIP PR submitter still making changes, not ready for review label Jul 23, 2024
@sabinala sabinala linked an issue Jul 23, 2024 that may be closed by this pull request
Add progress_hook to optimize #477
Closed
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@sabinala sabinala removed the request for review from anirban-chaudhuri July 23, 2024 18:32
@sabinala sabinala added awaiting review PR submitter awaiting code review from reviewer and removed WIP PR submitter still making changes, not ready for review labels Jul 31, 2024
@sabinala sabinala self-assigned this Jul 31, 2024
@Tom-Szendrey Tom-Szendrey mentioned this pull request Aug 7, 2024
Closed
3 tasks
SamWitty
SamWitty requested changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@SamWitty SamWitty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments, but overall this looks great. thanks!

pyciemss/ouu/ouu.py Outdated Show resolved Hide resolved
pyciemss/ouu/ouu.py Outdated Show resolved Hide resolved
docs/source/interfaces.ipynb Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is tqdm necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamWitty If there is another preferred way of displaying the progress bar, please let me know. This is the only way I am aware of.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub is showing that the only change in this PR is that tqdm is being imported. I'm not seeing it being used anywhere. If it's used, then definitely we should have the import. If not, we should remove it.

@SamWitty SamWitty added enhancement New feature or request awaiting response PR reviewer awaiting response from submitter and removed awaiting review PR submitter awaiting code review from reviewer labels Aug 27, 2024
@sabinala
Copy link
Contributor Author

@SamWitty if you're ok with keeping tqdm, this PR is ready for review. What do you think?

@anirban-chaudhuri
Copy link
Contributor

anirban-chaudhuri commented Aug 27, 2024 •
edited
Loading

I would wait on merging this PR. It is better to merge in the multiple constraints PR first just to make sure there are no merge conflicts (mostly because of changes to notebooks) and this one is a much smaller PR. Overall, it looks good though.

@SamWitty
Copy link
Contributor

@sabinala , could you please address the merge conflicts now that #596 is merged?

@sabinala sabinala added awaiting review PR submitter awaiting code review from reviewer and removed awaiting response PR reviewer awaiting response from submitter labels Aug 27, 2024
@sabinala sabinala requested a review from SamWitty August 27, 2024 20:10
SamWitty
SamWitty approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@SamWitty SamWitty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@SamWitty SamWitty merged commit 83c974a into main Aug 27, 2024
5 checks passed
@SamWitty SamWitty deleted the 477-add-progress_hook-to-optimize branch August 27, 2024 20:53
@Tom-Szendrey Tom-Szendrey mentioned this pull request Aug 28, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamWitty SamWitty SamWitty approved these changes

@anirban-chaudhuri anirban-chaudhuri Awaiting requested review from anirban-chaudhuri

Assignees

@sabinala sabinala

Labels
awaiting review PR submitter awaiting code review from reviewer enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add progress_hook to optimize
3 participants
@sabinala @anirban-chaudhuri @SamWitty